You should cache this using functools.cache

I've split the previous select_and_group into two functions aggregate_by and group_and_sum. I think the old function was doing too much and was misleading. This way group_and_sum alone is useful whenever you already have a DataFrame and just need deterministic aggregation.

@knikolla Is there a strong need for the output column to have a custom name? This function already only outputs the grouped and output columns, so without agg_name, there would not be any ambiguity.

a_summed = a.groupby([INVOICE_DATE_FIELD, PROJECT_FIELD], dropna=False, as_index=False)["Balance"].agg("sum")
print(a_summed.head())

  Invoice Month Project - Allocation  Balance
0       2024-03                False     6.43
1       2024-03                 True   403.30
2       2024-03                  NaN     3.30
3       2024-04                False  1000.00
4       2024-04                 True   510.00

I just don't want unneeded features for now.

I would suggest a stricter check, like an equality:

The test checks that the right type of expression was built, which I think is sufficient. Checking exact values is not really useful imho because if _row_filter garbles the values, group_and_sum and aggregate_by tests would catch it anyway since they operate end-to-end with real data.

The test checks that the right type of expression was built, which I think is sufficient. Checking exact values is not really useful

I disagree. This unit test is the only one letting us understand the expected behavior of _row_filter. We should know exactly the expected output should be, and in this is case it is not difficult to do so.

because if _row_filter garbles the values, group_and_sum and aggregate_by tests would catch it anyway since they operate end-to-end with real data.

get_invoice_dataframe, the only function that calls _row_filter, is heavily mocked in your test cases. None of your test cases deliberately test the filtering feature because we lack an integration test. Therefore, I'll argue none of the tests you presented adequately test _row_filter

Fixed

Have you considered unittest.mock, or pytest-mock to mock get_invoice_dataframe? Mocking will provide you with helper functions to capture how get_invoice_dataframe was called, removing the need for _fake_loader. There's a few examples of this mocking in this repo

This suggest would also help remove some mocking code you have further in your test cases, such as for class _FakeIcebergTable

I think we should keep the _FakeIcebergTable. I believe having code that is understandable is better.

_FakeIcebergTable explicitly documents the .scan().select().to_pandas() chain the code depends on. Which is very useful to understand the context which is something that a MagicMock will abstract away.

The purpose of the test case is to see if get_invoice_dataframe emits a warning if the filtered dataframe is empty. It is explicitly not concerned with the internals of the iceberg table, which is why this is mocked out in the first place. A developer would review a test case to understand the expected behavior, not the internals of how that is done.

Looking at it a bit more, I would argue asserting that the output equals the input is moot here, since you setup the mock to return the same dataframe. If we wanted to test the pyiceberg code, we would need an integration test, which can be a part of this PR or a seperate one. @knikolla Thoughts?